Add cpu vendor to snapshots#1587
Conversation
ludfjig
added
the
kind/enhancement
ludfjig
requested review from
andreiltd,
danbugs,
dblnz,
devigned,
jprendes,
jsturtevant,
simongdavies,
squillace and
syntactically
as code owners
There was a problem hiding this comment.
Pull request overview
This PR tightens snapshot portability rules by recording the host CPU vendor in the OCI snapshot config (and mirroring it into the manifest descriptor annotations) so snapshots taken on one CPU vendor (e.g., Intel) are rejected when loaded on a different vendor (e.g., AMD), reducing the risk of resuming incompatible CPU state.
Changes:
- Add
cpu_vendorto the snapshot config and validate it at load time. - Emit a new
dev.hyperlight.snapshot.cpu.vendoradvisory annotation on the manifest descriptor. - Update documentation and add a test to ensure vendor mismatches are rejected.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/snapshot/file/mod.rs | Writes the CPU vendor annotation and includes cpu_vendor in the saved config. |
| src/hyperlight_host/src/sandbox/snapshot/file/media_types.rs | Adds the new CPU vendor annotation key and updates annotation docs. |
| src/hyperlight_host/src/sandbox/snapshot/file/config.rs | Introduces CpuVendor and enforces CPU vendor matching during load validation. |
| src/hyperlight_host/src/sandbox/snapshot/file_tests.rs | Adds a unit test asserting vendor mismatches are rejected. |
| docs/snapshot-oci-format.md | Documents the new CPU vendor binding and annotation. |
jprendes
left a comment
jprendes
left a comment
There was a problem hiding this comment.
LGTM, but I think the copilot comment is on point, we don't want to accidentally crash on Windows on ARM.
danbugs
left a comment
danbugs
left a comment
There was a problem hiding this comment.
Mostly LGTM, but I think you still need to address the copilot review. Plus—what are your thoughts on providing an escape hatch on this requirement? For example, a skip_cpu_vendor_check option (or similar) on the load path that defaults to false but lets callers explicitly opt out when they've validated cross-vendor compatibility for their use case. That way the default is strict, but users aren't hard-blocked if they know what they're doing—and it gives us a natural knob to flip when we're ready to relax the restriction across the board.
ludfjig
force-pushed
the
cpu-vendor
branch
3 times, most recently
from
1f80796 to
1f069c2
Compare
I don't think I am a fan for this release. We should just do the work to allow it (or not) and fix it and allow it |
…ding Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Error on loading intel snapshots on amd and vice versa (tried my best for aarch64 but not sure if correct please review). The goal is to relax this requirement later, but will need more investigation.